-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[libc++] Add __pointer_int_pair #94324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis is an implementation detail for Full diff: https://github.com/llvm/llvm-project/pull/94324.diff 6 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index cfe1f44777bca..289baf2338256 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -857,6 +857,7 @@ set(files
__utility/no_destroy.h
__utility/pair.h
__utility/piecewise_construct.h
+ __utility/pointer_int_pair.h
__utility/priority_tag.h
__utility/private_constructor_tag.h
__utility/rel_ops.h
diff --git a/libcxx/include/__utility/pointer_int_pair.h b/libcxx/include/__utility/pointer_int_pair.h
new file mode 100644
index 0000000000000..9c64b023c60e6
--- /dev/null
+++ b/libcxx/include/__utility/pointer_int_pair.h
@@ -0,0 +1,154 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___UTILITY_POINTER_INT_PAIR_H
+#define _LIBCPP___UTILITY_POINTER_INT_PAIR_H
+
+#include <__assert>
+#include <__bit/bit_log2.h>
+#include <__concepts/derived_from.h>
+#include <__config>
+#include <__tuple/tuple_element.h>
+#include <__tuple/tuple_size.h>
+#include <__type_traits/is_pointer.h>
+#include <__type_traits/is_unsigned.h>
+#include <__type_traits/is_void.h>
+#include <__type_traits/remove_pointer.h>
+#include <__utility/swap.h>
+#include <cstddef>
+#include <cstdint>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+#if _LIBCPP_STD_VER >= 23
+
+// A __pointer_int_pair is a pair of a pointer and an integral type. The lower bits of the pointer that are free
+// due to the alignment requirement of the pointee are used to store the integral type.
+//
+// This imposes a constraint on the number of bits available for the integral type -- the integral type can use
+// at most log2(alignof(T)) bits. This technique allows storing the integral type without additional storage
+// beyond that of the pointer itself, at the cost of some bit twiddling.
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template <class>
+struct _PointerLikeTraits;
+
+template <class _Tp>
+ requires(!is_void_v<_Tp>)
+struct _PointerLikeTraits<_Tp*> {
+ static constexpr size_t __low_bits_available = std::__bit_log2(alignof(_Tp));
+
+ static _LIBCPP_HIDE_FROM_ABI uintptr_t __to_uintptr(_Tp* __ptr) { return reinterpret_cast<uintptr_t>(__ptr); }
+ static _LIBCPP_HIDE_FROM_ABI _Tp* __to_pointer(uintptr_t __ptr) { return reinterpret_cast<_Tp*>(__ptr); }
+};
+
+template <class _Tp>
+ requires is_void_v<_Tp>
+struct _PointerLikeTraits<_Tp*> {
+ static constexpr size_t __low_bits_available = 0;
+
+ static _LIBCPP_HIDE_FROM_ABI uintptr_t __to_uintptr(_Tp* __ptr) { return reinterpret_cast<uintptr_t>(__ptr); }
+ static _LIBCPP_HIDE_FROM_ABI _Tp* __to_pointer(uintptr_t __ptr) { return reinterpret_cast<_Tp*>(__ptr); }
+};
+
+enum class __integer_width : size_t {};
+
+template <class _Pointer, class _IntType, __integer_width __int_bit_count>
+class __pointer_int_pair {
+ using _PointerTraits = _PointerLikeTraits<_Pointer>;
+
+ static constexpr auto __int_width = static_cast<size_t>(__int_bit_count);
+
+ static_assert(__int_width <= _PointerTraits::__low_bits_available,
+ "Not enough bits available for requested bit count");
+ static_assert(is_integral_v<_IntType>, "_IntType has to be an integral type");
+ static_assert(is_unsigned_v<_IntType>, "__pointer_int_pair doesn't work for signed types");
+
+ static constexpr size_t __extra_bits = _PointerTraits::__low_bits_available - __int_width;
+ static constexpr uintptr_t __int_mask = static_cast<uintptr_t>(1 << _PointerTraits::__low_bits_available) - 1;
+ static constexpr uintptr_t __ptr_mask = ~__int_mask;
+
+ uintptr_t __value_ = 0;
+
+public:
+ __pointer_int_pair() = default;
+ __pointer_int_pair(const __pointer_int_pair&) = default;
+ __pointer_int_pair(__pointer_int_pair&&) = default;
+ __pointer_int_pair& operator=(const __pointer_int_pair&) = default;
+ __pointer_int_pair& operator=(__pointer_int_pair&&) = default;
+ ~__pointer_int_pair() = default;
+
+ _LIBCPP_HIDE_FROM_ABI __pointer_int_pair(_Pointer __ptr_value, _IntType __int_value)
+ : __value_(_PointerTraits::__to_uintptr(__ptr_value) | (__int_value << __extra_bits)) {
+ _LIBCPP_ASSERT_INTERNAL((__int_value & (__int_mask >> __extra_bits)) == __int_value, "integer is too large!");
+ _LIBCPP_ASSERT_INTERNAL(
+ (_PointerTraits::__to_uintptr(__ptr_value) & __ptr_mask) == _PointerTraits::__to_uintptr(__ptr_value),
+ "Pointer alignment is too low!");
+ }
+
+ _LIBCPP_HIDE_FROM_ABI _IntType __get_value() const { return (__value_ & __int_mask) >> __extra_bits; }
+ _LIBCPP_HIDE_FROM_ABI _Pointer __get_ptr() const { return _PointerTraits::__to_pointer(__value_ & __ptr_mask); }
+
+ template <class>
+ friend struct _PointerLikeTraits;
+};
+
+template <class _Pointer, __integer_width __int_bit_count, class _IntType>
+struct _PointerLikeTraits<__pointer_int_pair<_Pointer, _IntType, __int_bit_count>> {
+private:
+ using _PointerIntPair = __pointer_int_pair<_Pointer, _IntType, __int_bit_count>;
+
+ static_assert(_PointerLikeTraits<_Pointer>::__low_bits_available >= static_cast<size_t>(__int_bit_count));
+
+public:
+ static constexpr size_t __low_bits_available =
+ _PointerLikeTraits<_Pointer>::__low_bits_available - static_cast<size_t>(__int_bit_count);
+
+ static _LIBCPP_HIDE_FROM_ABI uintptr_t __to_uintptr(_PointerIntPair __ptr) { return __ptr.__value_; }
+
+ static _LIBCPP_HIDE_FROM_ABI _PointerIntPair __to_pointer(uintptr_t __ptr) {
+ _PointerIntPair __tmp{};
+ __tmp.__value_ = __ptr;
+ return __tmp;
+ }
+};
+
+// Make __pointer_int_pair tuple-like
+
+template <class _Pointer, class _IntType, __integer_width __int_bit_count>
+struct tuple_size<__pointer_int_pair<_Pointer, _IntType, __int_bit_count>> : integral_constant<size_t, 2> {};
+
+template <class _Pointer, class _IntType, __integer_width __int_bit_count>
+struct tuple_element<0, __pointer_int_pair<_Pointer, _IntType, __int_bit_count>> {
+ using type = _Pointer;
+};
+
+template <class _Pointer, class _IntType, __integer_width __int_bit_count>
+struct tuple_element<1, __pointer_int_pair<_Pointer, _IntType, __int_bit_count>> {
+ using type = _IntType;
+};
+
+template <size_t __i, class _Pointer, class _IntType, __integer_width __int_bit_count>
+_LIBCPP_HIDE_FROM_ABI auto get(__pointer_int_pair<_Pointer, _IntType, __int_bit_count> __pair) {
+ if constexpr (__i == 0) {
+ return __pair.__get_ptr();
+ } else if constexpr (__i == 1) {
+ return __pair.__get_value();
+ } else {
+ static_assert(__i == 0, "Index out of bounds");
+ }
+}
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_STD_VER >= 23
+
+#endif // _LIBCPP___UTILITY_POINTER_INT_PAIR_H
diff --git a/libcxx/test/libcxx/utilities/pointer_int_pair/assert.constructor.pass.cpp b/libcxx/test/libcxx/utilities/pointer_int_pair/assert.constructor.pass.cpp
new file mode 100644
index 0000000000000..319455f25a482
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/pointer_int_pair/assert.constructor.pass.cpp
@@ -0,0 +1,44 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// REQUIRES: has-unix-headers
+// REQUIRES: libcpp-hardening-mode=debug
+// XFAIL: availability-verbose_abort-missing
+
+#include "test_macros.h"
+
+TEST_DIAGNOSTIC_PUSH
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__utility/pointer_int_pair.h>
+TEST_DIAGNOSTIC_POP
+
+#include <cassert>
+
+#include "check_assertion.h"
+
+struct [[gnu::packed]] Packed {
+ char c;
+ int i;
+};
+
+int main(int, char**) {
+ TEST_LIBCPP_ASSERT_FAILURE(
+ (std::__pointer_int_pair<int*, size_t, std::__integer_width{1}>{nullptr, 2}), "integer is too large!");
+
+ TEST_DIAGNOSTIC_PUSH
+ TEST_CLANG_DIAGNOSTIC_IGNORED("-Waddress-of-packed-member") // That's what we're trying to test
+ TEST_GCC_DIAGNOSTIC_IGNORED("-Waddress-of-packed-member")
+ alignas(int) Packed p;
+ TEST_LIBCPP_ASSERT_FAILURE(
+ (std::__pointer_int_pair<int*, size_t, std::__integer_width{1}>{&p.i, 0}), "Pointer alignment is too low!");
+ TEST_DIAGNOSTIC_POP
+
+ return 0;
+}
diff --git a/libcxx/test/libcxx/utilities/pointer_int_pair/constinit.verify.cpp b/libcxx/test/libcxx/utilities/pointer_int_pair/constinit.verify.cpp
new file mode 100644
index 0000000000000..e06a378bf11df
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/pointer_int_pair/constinit.verify.cpp
@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Ensure that __pointer_int_pair cannot be constant initialized with a value.
+// This would mean that the constructor is `constexpr`, which should only be
+// possible with compiler magic.
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+#include <__utility/pointer_int_pair.h>
+
+template <class Ptr, class UnderlyingType>
+using single_bit_pair = std::__pointer_int_pair<Ptr, UnderlyingType, std::__integer_width{1}>;
+
+constinit int ptr = 0;
+constinit single_bit_pair<int*, size_t> continitiable_pointer_int_pair_values{&ptr, 0}; // expected-error {{variable does not have a constant initializer}}
diff --git a/libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp b/libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp
new file mode 100644
index 0000000000000..6ab15d9233663
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp
@@ -0,0 +1,104 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+#include <__utility/pointer_int_pair.h>
+#include <cassert>
+
+template <class Ptr, class UnderlyingType>
+using single_bit_pair = std::__pointer_int_pair<Ptr, UnderlyingType, std::__integer_width{1}>;
+
+template <class Ptr, class UnderlyingType>
+using two_bit_pair = std::__pointer_int_pair<Ptr, UnderlyingType, std::__integer_width{2}>;
+
+constinit single_bit_pair<int*, size_t> continitiable_pointer_int_pair;
+
+int main(int, char**) {
+ { // __point_int_pair() constructor
+ single_bit_pair<int*, size_t> pair = {};
+ assert(pair.__get_value() == 0);
+ assert(pair.__get_ptr() == nullptr);
+ }
+
+ { // __pointer_int_pair(pointer, int) constructor
+ single_bit_pair<int*, size_t> pair(nullptr, 1);
+ assert(pair.__get_value() == 1);
+ assert(pair.__get_ptr() == nullptr);
+ }
+
+ { // pointer is correctly packed/unpacked (with different types and values)
+ int i;
+ single_bit_pair<int*, size_t> pair(&i, 0);
+ assert(pair.__get_value() == 0);
+ assert(pair.__get_ptr() == &i);
+ }
+ {
+ int i;
+ two_bit_pair<int*, size_t> pair(&i, 2);
+ assert(pair.__get_value() == 2);
+ assert(pair.__get_ptr() == &i);
+ }
+ {
+ short i;
+ single_bit_pair<short*, size_t> pair(&i, 1);
+ assert(pair.__get_value() == 1);
+ assert(pair.__get_ptr() == &i);
+ }
+
+ { // check that a __pointer_int_pair<__pointer_int_pair> works
+ int i;
+ single_bit_pair<single_bit_pair<int*, size_t>, size_t> pair({&i, 1}, 0);
+ assert(pair.__get_value() == 0);
+ assert(pair.__get_ptr().__get_ptr() == &i);
+ assert(pair.__get_ptr().__get_value() == 1);
+ }
+
+ { // check that the tuple protocol is correctly implemented
+ int i;
+ two_bit_pair<int*, size_t> pair{&i, 3};
+ auto [ptr, value] = pair;
+ assert(ptr == &i);
+ assert(value == 3);
+ }
+
+ { // check that the (pointer, int) constructor is implicit
+ int i;
+ two_bit_pair<int*, size_t> pair = {&i, 3};
+ assert(pair.__get_ptr() == &i);
+ assert(pair.__get_value() == 3);
+ }
+
+ { // check that overaligned types work as expected
+ struct alignas(32) Overaligned {
+ int i;
+ };
+
+ Overaligned i;
+ std::__pointer_int_pair<Overaligned*, size_t, std::__integer_width{4}> pair = {&i, 13};
+ assert(pair.__get_ptr() == &i);
+ assert(pair.__get_value() == 13);
+ }
+
+ { // check that types other than size_t work as well
+ int i;
+ single_bit_pair<int*, bool> pair = {&i, true};
+ assert(pair.__get_ptr() == &i);
+ assert(pair.__get_value());
+ static_assert(std::is_same_v<decltype(pair.__get_value()), bool>);
+ }
+ {
+ int i;
+ single_bit_pair<int*, unsigned char> pair = {&i, 1};
+ assert(pair.__get_ptr() == &i);
+ assert(pair.__get_value() == 1);
+ static_assert(std::is_same_v<decltype(pair.__get_value()), unsigned char>);
+ }
+
+ return 0;
+}
diff --git a/libcxx/test/libcxx/utilities/pointer_int_pair/static_asserts.verify.cpp b/libcxx/test/libcxx/utilities/pointer_int_pair/static_asserts.verify.cpp
new file mode 100644
index 0000000000000..544769839679c
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/pointer_int_pair/static_asserts.verify.cpp
@@ -0,0 +1,23 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+#include "test_macros.h"
+
+TEST_DIAGNOSTIC_PUSH
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__utility/pointer_int_pair.h>
+TEST_DIAGNOSTIC_POP
+
+// expected-error-re@*:* {{{{static_assert|static assertion}} failed {{.*}}Not enough bits available for requested bit count}}
+std::__pointer_int_pair<char*, size_t, std::__integer_width{2}> ptr1; // expected-note {{here}}
+// expected-error-re@*:* {{{{static_assert|static assertion}} failed {{.*}}_IntType has to be an integral type}}
+std::__pointer_int_pair<int*, int*, std::__integer_width{2}> ptr2; // expected-note {{here}}
+// expected-error-re@*:* {{{{static_assert|static assertion}} failed {{.*}}__pointer_int_pair doesn't work for signed types}}
+std::__pointer_int_pair<int*, signed, std::__integer_width{2}> ptr3; // expected-note {{here}}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
21fce33
to
98c61df
Compare
Can you point me to the code, in a PR or otherwise, that makes use of this code? I don't think we should add code before we use it. My concern is that [You aren't going to need it (N When in doubt, keep behaviors separate until enough common patterns emerge over time that justify the coupling. On a small scale, managing duplication can be simpler than resolving a premature abstraction’s complexity. In early stages of development, tolerate a little duplication and wait to abstract. 1 Could we move forward implementing move_only_function, then after it's implemented, see what actually needs to be done? This is not the only unused abstraction that's been committed to libc++ for move_only_function recently. When looking for the implementation of |
I would like to block this until I cannot meaningfully review the utility, purpose, and correctness of this class without at least one user. What is the status of |
I generally agree with the statement "don't try to DRY your code too early", however
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am generally quite happy with this patch, and IIRC it has been reviewed (and maybe even approved) on Phabricator last year.
Let's see what @EricWF thinks about introducing this now so we can use it immediately after in move_only_function
, but other than that this LGTM.
_LIBCPP_BEGIN_NAMESPACE_STD | ||
|
||
template <class> | ||
struct _PointerLikeTraits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/foonathan/tiny might be worth taking a look at. _PointerLikeTraits
is probably good enough for now, but if we want to generalize we might want to change the name of the trait.
libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/libcxx/utilities/pointer_int_pair/static_asserts.verify.cpp
Outdated
Show resolved
Hide resolved
@philnik777 Can you upload the patch for |
Out of interest, how is this useful in the implementation of move_only_function ? I was thinking of union { const vtable* vtable_ Where vtable contains We don’t seem to need a bool because the vtable function implementation should deal with question whether or not the object is in the buffer or in the heap |
177070b
to
b8a792a
Compare
|
@philnik777 Thanks for putting that up. I've yet to convince myself that an abstraction this heavy weight is generally useful. I'm worried because it's usefulness is very limited to basically In other use cases, where the value we pack is more than Basically, we need to control own the pointer type & value to prevent issues like with the If this is blocking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we discovered that this type is not needed (and somewhat harmful to the optimizer)
|
||
// Make __pointer_int_pair tuple-like | ||
|
||
template <class _Pointer, class _IntType, __integer_width __int_bit_count> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this to be tuple-like
?
It's a bunch of instantiations we should really try to avoid. (I'm considering how much the bloat of __compressed_pair
hurt us)
5780820
to
d28c822
Compare
d28c822
to
c9cbd37
Compare
68f4c10
to
c2fb284
Compare
c2fb284
to
4b4b6e4
Compare
This is an implementation detail for
move_only_function
.